Skip to content

docs: add decentralized masternode shares draft DIP#185

Open
thepastaclaw wants to merge 18 commits into
dashpay:masterfrom
thepastaclaw:docs/dip-decentralized-mn-shares
Open

docs: add decentralized masternode shares draft DIP#185
thepastaclaw wants to merge 18 commits into
dashpay:masterfrom
thepastaclaw:docs/dip-decentralized-mn-shares

Conversation

@thepastaclaw

@thepastaclaw thepastaclaw commented Jun 20, 2026

Copy link
Copy Markdown

Summary

Adds a draft, unnumbered DIP for Decentralized Masternode Shares.

The draft builds on DIP-0003, DIP-0023, DIP-0026 v4, and DIP-0028 to specify shared-collateral masternode registration, per-share reward distribution, participant-authorized updates, and constrained dissolution.

Also adds:

  • README discoverability for the draft DIP using the TBD placeholder until editor assignment.
  • Proposed DIP-0002 special transaction type registry rows for:
    • 10ProDisTx
    • 11ProUpShareTx
    • 12ProUpSharedRegTx
  • cspell vocabulary needed by the new draft and touched README text.

Dependency / review note

This PR is intentionally draft-blocked on #184, which updates DIP-0026 to the v4 provider-payload semantics this draft references. The shared-masternode branch has been reviewed against that DIP-0026 v4 base; once #184 lands, this PR should become repository-consistent without bundling the DIP-0026 changes here.

Open design feedback to resolve before ready-for-review: whether shared-collateral semantics should remain a separate provider payload version (SharedCollateral = 5, as currently drafted) or be folded into a single version that supports both shared and non-shared masternodes via an explicit flag or share-count discriminator.

Notes for editors/reviewers

  • The DIP intentionally remains DIP: TBD and uses the alias filename dip-decentralized-masternode-shares.md; authors must not self-assign DIP numbers.
  • Transaction type values 10, 11, and 12 are registered here as proposed values for this draft. If any value is consumed before merge, this DIP must be updated to the next free values before merge.
  • Deployment parameters, including the final DIP-0023 deployment name and SHARED_MIN_SHARE_DUFFS, remain explicitly open before activation.
  • This is spec/docs only. No Dash Core implementation is included in this PR.

Validation

  • git diff --check
  • npx cspell --no-progress README.md dip-decentralized-masternode-shares.md dip-0002/special-transactions.md
  • npx markdownlint-cli2 README.md dip-decentralized-masternode-shares.md dip-0002/special-transactions.md --config .markdownlint.json
  • Mandatory pre-PR code-review gate against the intended DIP-0026 v4 base (fix/dip26-review-followup...docs/dip-decentralized-mn-shares): ship

Summary by CodeRabbit

  • Documentation
    • Refined the Dash Improvement Proposals (DIPs) introduction wording and updated the “Dash Improvement Proposal Summary” table.
    • Added a new draft DIP: Decentralized Masternode Shares.
    • Expanded special transaction documentation with reformatting and new proposed transaction types 10–12 tied to the new draft.
    • Improved documentation spell-check resources by adding accepted spelling entries and additional commonly used terms.

@coderabbitai

coderabbitai Bot commented Jun 20, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: e720f32c-9aec-48a2-a789-1e81c57d5594

📥 Commits

Reviewing files that changed from the base of the PR and between 1b778bc and 51ec7ac.

📒 Files selected for processing (1)
  • dip-decentralized-masternode-shares.md
🚧 Files skipped from review as they are similar to previous changes (1)
  • dip-decentralized-masternode-shares.md

📝 Walkthrough

Walkthrough

A new DIP draft, "Decentralized Masternode Shares," is added as dip-decentralized-masternode-shares.md (1,815 lines), specifying a shared-collateral masternode model with ProTxVersion::SharedCollateral = 5 and three new special transaction types (ProDisTx/10, ProUpShareTx/11, ProUpSharedRegTx/12). The README index and DIP-0002 transaction table are updated to reference these additions, and the spell-checker word list is extended.

Changes

Decentralized Masternode Shares DIP

Layer / File(s) Summary
README index, DIP-0002 tx table, and word list
README.md, dip-0002/special-transactions.md, project-words.txt
Adds a Draft/TBD DIP row in the README summary table, registers types 10–12 (ProDisTx, ProUpShareTx, ProUpSharedRegTx) as Proposed in the DIP-0002 special transaction table with a minor italics fix on "payload version," and extends the spell-checker vocabulary with terminology used in the DIP.
DIP front matter, scope, abstract, terminology, and constants
dip-decentralized-masternode-shares.md
Adds document metadata (title, authors, status, dependency on DIP-0026), full table of contents, purpose/scope overview, core terminology (shared-collateral, participant, protected outpoints), numeric constants (participant and share limits, penalty bounds, reward precision), ProTxVersion::SharedCollateral = 5, and the three new special transaction type integer values.
CollateralShare structure and ProRegTx v5 registration validation
dip-decentralized-masternode-shares.md
Defines the CollateralShare structure with immutability constraints and key/script reuse restrictions; specifies the v5 shared-collateral output script template (OP_TRUE) and consensus covenant protection rules; documents the full ProRegTx v5 payload layout including participant registration consent digest construction, joinSig authorization verification, and validation conditions.
Shared-collateral update transactions ProUpShareTx and ProUpSharedRegTx
dip-decentralized-masternode-shares.md
Specifies ProUpShareTx (type 11) to update one share's rewardScript and ProUpSharedRegTx (type 12) to unanimously update shared-registrar fields; includes payload shapes, authorization digest construction, signature cardinality/order rules, validation constraints, and operator-update reset behavior.
Reward distribution and ProDisTx dissolution
dip-decentralized-masternode-shares.md
Specifies DistributeByWeight share-weighted reward allocation with deterministic rounding and overflow-safe arithmetic; defines ProDisTx (type 10) end-to-end including authorization digest, penalty computation, signature cardinality rules for unilateral vs. unanimous modes, output-covenant construction (exact share-order outputs with optional actor-slot omission), fee accounting, same-block prohibition, and state-effect rules.
Collateral spend enforcement and consensus integration
dip-decentralized-masternode-shares.md
Introduces explicit collateral spend enforcement rules for mempool and block connection; defines protected outpoint tracking sets (registered, same-block pending, mempool pending); specifies same-block pending output rejection, deterministic masternode removal timing tied to validated later-block ProDisTx, and deterministic restoration on block disconnection for reorg handling.
Deterministic state extension, governance, and SML/filter behavior
dip-decentralized-masternode-shares.md
Extends deterministic masternode state for v5 with share vectors and frozen penalty parameters; mandates state-diff full-replacement encoding for share-vector changes; updates uniqueness/index rules to incorporate participant owner keys; specifies SML metadata exclusion and filter element extensions; defines governance as single per-masternode keyIDVoting updated only via ProUpSharedRegTx with unanimous signatures.
Deployment gating, rationale, test cases, and security considerations
dip-decentralized-masternode-shares.md
Adds deployment/compatibility guidance (draft status, pre/post-activation validity rules, no conversion between shared/non-shared modes); provides comprehensive rationale for design constraints (explicit consent, dissolution digest coverage, penalty bounds, enforcement timing, same-block prohibition); includes non-normative test cases for registration/update/dissolution and reorg handling; documents implementation notes, security considerations (trust model, compromise scenarios, griefing/dust/light-client/replay separation), open issues, and copyright.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: adding a draft DIP document for Decentralized Masternode Shares, which is the primary content of this PR.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@thepastaclaw

Copy link
Copy Markdown
Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 20, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@thepastaclaw

thepastaclaw commented Jun 20, 2026

Copy link
Copy Markdown
Author

✅ Review complete (commit 51ec7ac)

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Two convergent blocking findings on the v5 shared-masternode draft: the registration-consent sharesWithoutJoinSigs serialization is consensus-ambiguous, and the ProUpShareTx / ProUpSharedRegTx authorization digests sign nLockTime without binding input sequences (asymmetric with the registration and dissolution digests that already commit to sequences). One additional blocking issue: the draft claims DIP-0026 occupies provider payload version 4, but DIP-0026 in this repo currently specifies version 2, leaving the version ladder for SharedCollateral = 5 undefined. Two nitpicks on a wording inconsistency for the ProUpShareTx key-reuse rule and a logically inconsistent reorg test case.

🔴 3 blocking | 💬 2 nitpick(s)

🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `dip-decentralized-masternode-shares.md`:
- [BLOCKING] dip-decentralized-masternode-shares.md:514-516: `sharesWithoutJoinSigs` serialization is ambiguous and consensus-critical
  The text says `sharesWithoutJoinSigs` is the concatenation of every CollateralShare with the `joinSig` field `omitted (treated as a zero-length byte string)`. Two reasonable readings produce different byte sequences and therefore different SharedRegConsentHash values:

  1. The `joinSig` field is omitted entirely — no bytes at all in the concatenation.
  2. The `joinSig` field is serialized as a zero-length compact-size bytestring — i.e., the single byte `0x00` (compact-size length 0, followed by no payload bytes), preserving the field's framing.

  `CollateralShare` (line 339) defines `joinSig` as `vector<unsigned char>  (compact-size length-prefixed, ...)`, which makes interpretation (2) the natural literal reading of `serialize(share)` with an empty vector — yet the word `omitted` suggests (1). Because every participant signs this digest and consensus recomputes it to verify `joinSig`, divergent implementations would either reject otherwise-valid v5 registrations or, once one implementation is patched, cause a chain split. The DIP must pin one encoding normatively and ideally show pseudocode (e.g., `serialize(share) with joinSig replaced by compact_size(0)` vs. `serialize(share) skipping the joinSig field entirely`).
- [BLOCKING] dip-decentralized-masternode-shares.md:573-636: ProUpShareTx and ProUpSharedRegTx digests sign nLockTime without binding input sequences
  The registration digest (line 468) commits to `sequencesHash`, and the dissolution digest (line 775) explicitly commits to `tx.vin[0].nSequence`; in both cases the DIP states the reason is to stop a coordinator/fee-payer from changing finality, RBF, or `nLockTime` semantics after participants sign (lines 500–504, 783–787). The ProUpShareTx digest (lines 573–580) and the ProUpSharedRegTx digest (lines 628–636) commit only to `inputsHash`, `tx.nLockTime`, and `tx.nVersion`/`tx.nType`; they bind no input `nSequence`.

  Because `nLockTime` is only enforced when at least one input has a non-final sequence, a party that controls or replaces the funding input signatures (which need not be SIGHASH_ALL/AC-payable in a way that commits to sequences) can flip an input to a final sequence after the participant signs, effectively neutralising the signed `nLockTime` (e.g., causing a future-dated reward-script update or shared-registrar update to take effect immediately). The risk is particularly acute for ProUpSharedRegTx, where every participant signs but a separate broadcaster/fee-payer typically supplies the funding inputs.

  Mirror the registration/dissolution model by adding `sequencesHash` (or an enumerated commitment to each input's `nSequence`) to both update digests, or — if the omission is deliberate because these updates do not bind finality — add a normative one-line note explaining why and remove `nLockTime` from these digests so implementers do not believe the spec enforces something it does not.
- [BLOCKING] dip-decentralized-masternode-shares.md:111-116: Provider payload versioning conflicts with the referenced DIP-0026
  This draft normatively claims `DIP-0026 introduces provider transaction payload version 4 (MultiPayout)` and then allocates `SharedCollateral = 5` for v5. The DIP-0026 document checked into this repository (`dip-0026.md` line 46) says it introduces `version 2` of ProRegTx/ProUpRegTx with `payoutShares`/`payoutSharesSize`, and makes no claim about versions 3 or 4. As a result, implementers reading only the merged DIPs cannot determine whether DIP-0026 is v2 or v4, what occupies version 3, or whether `SharedCollateral` should actually be version 3 (next free after DIP-0026) rather than version 5.

  The provider payload version is a consensus selector, so this is the kind of registry mismatch the review guidance flags as blocking. Either amend DIP-0026 in this repo to match (renumber its payload to v4 and reserve v3 / specify what it is), or change this DIP's version allocation to be consistent with DIP-0026 as merged. The 'Open Issues' section does not currently cover this dependency, so add it there or in 'Relationship to DIP-0026' with the resolution path.

Comment thread dip-decentralized-masternode-shares.md Outdated
Comment on lines +514 to +516
* `sharesWithoutJoinSigs` is the concatenation of every `CollateralShare`
in share order with the `joinSig` field omitted (treated as a zero-length
byte string) so that signatures are not self-referential.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Blocking: sharesWithoutJoinSigs serialization is ambiguous and consensus-critical

The text says sharesWithoutJoinSigs is the concatenation of every CollateralShare with the joinSig field omitted (treated as a zero-length byte string). Two reasonable readings produce different byte sequences and therefore different SharedRegConsentHash values:

  1. The joinSig field is omitted entirely — no bytes at all in the concatenation.
  2. The joinSig field is serialized as a zero-length compact-size bytestring — i.e., the single byte 0x00 (compact-size length 0, followed by no payload bytes), preserving the field's framing.

CollateralShare (line 339) defines joinSig as vector<unsigned char> (compact-size length-prefixed, ...), which makes interpretation (2) the natural literal reading of serialize(share) with an empty vector — yet the word omitted suggests (1). Because every participant signs this digest and consensus recomputes it to verify joinSig, divergent implementations would either reject otherwise-valid v5 registrations or, once one implementation is patched, cause a chain split. The DIP must pin one encoding normatively and ideally show pseudocode (e.g., serialize(share) with joinSig replaced by compact_size(0) vs. serialize(share) skipping the joinSig field entirely).

source: ['claude']

Comment on lines +573 to +636
```text
SHA256d("DashSharedMNUpShare" ||
chainGenesisHash ||
LE16(tx.nVersion) || LE16(tx.nType) || LE32(tx.nLockTime) ||
inputsHash ||
LE16(payload.nVersion) ||
proTxHash || LE16(shareIndex) || newRewardScript)
```

`chainGenesisHash` is the 32-byte genesis block hash of the network
validating the transaction. `payload.nVersion` is the ProUpShareTx payload
version (currently `1`).

A valid `ProUpShareTx` replaces `shares[shareIndex].rewardScript` in the
deterministic masternode state. It does not revive a PoSe-banned masternode
and does not affect any other field.

#### ProUpSharedRegTx (type 12)

A `ProUpSharedRegTx` updates fields that affect all participants. In this
DIP the mutable shared-registrar fields are `pubKeyOperator`, `keyIDVoting`,
and `nOperatorReward`.

```text
CProUpSharedRegTx {
nVersion : uint16_t // MUST be 1 (payload version of this new special tx type)
proTxHash : uint256
pubKeyOperator : CBLSPublicKey // basic scheme
keyIDVoting : CKeyID
nOperatorReward : uint16_t
inputsHash : uint256
vchSigs : vector<vector<unsigned char>> // exactly shares.size() entries, in share order
}
```

As with `ProUpShareTx`, this `nVersion` is the independent payload version
of the new `TRANSACTION_PROVIDER_UPDATE_SHARED_REGISTRAR` special
transaction and is unrelated to `ProTxVersion::SharedCollateral = 5`.

Validation:

1. The masternode identified by `proTxHash` MUST exist and MUST be a v5
masternode.
2. `pubKeyOperator` MUST be a valid basic-scheme BLS public key and MUST
NOT collide with the operator key of any other registered masternode.
3. `keyIDVoting` MUST NOT equal any `shares[i].ownerKey`.
4. No current `refundScript` and no current effective `rewardScript` may be a
P2PKH script paying to `keyIDVoting`. For this check, an empty
`rewardScript` is interpreted as the corresponding `refundScript`.
5. `nOperatorReward <= 10000`.
6. `inputsHash` MUST equal `CalcTxInputsHash(tx)`.
7. `vchSigs.size()` MUST equal `shares.size()`.
8. For each `i` in `[0, shares.size())`, `vchSigs[i]` MUST be a valid
ECDSA compact signature by `shares[i].ownerKey` over:

```text
SHA256d("DashSharedMNUpSharedReg" ||
chainGenesisHash ||
LE16(tx.nVersion) || LE16(tx.nType) || LE32(tx.nLockTime) ||
inputsHash ||
LE16(payload.nVersion) ||
proTxHash || pubKeyOperatorSerialized ||
keyIDVoting || LE16(nOperatorReward))
```

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Blocking: ProUpShareTx and ProUpSharedRegTx digests sign nLockTime without binding input sequences

The registration digest (line 468) commits to sequencesHash, and the dissolution digest (line 775) explicitly commits to tx.vin[0].nSequence; in both cases the DIP states the reason is to stop a coordinator/fee-payer from changing finality, RBF, or nLockTime semantics after participants sign (lines 500–504, 783–787). The ProUpShareTx digest (lines 573–580) and the ProUpSharedRegTx digest (lines 628–636) commit only to inputsHash, tx.nLockTime, and tx.nVersion/tx.nType; they bind no input nSequence.

Because nLockTime is only enforced when at least one input has a non-final sequence, a party that controls or replaces the funding input signatures (which need not be SIGHASH_ALL/AC-payable in a way that commits to sequences) can flip an input to a final sequence after the participant signs, effectively neutralising the signed nLockTime (e.g., causing a future-dated reward-script update or shared-registrar update to take effect immediately). The risk is particularly acute for ProUpSharedRegTx, where every participant signs but a separate broadcaster/fee-payer typically supplies the funding inputs.

Mirror the registration/dissolution model by adding sequencesHash (or an enumerated commitment to each input's nSequence) to both update digests, or — if the omission is deliberate because these updates do not bind finality — add a normative one-line note explaining why and remove nLockTime from these digests so implementers do not believe the spec enforces something it does not.

source: ['claude', 'codex']

Comment thread dip-decentralized-masternode-shares.md Outdated
Comment on lines +111 to +116
DIP-0026 introduces provider transaction payload version `4` (`MultiPayout`).
Under DIP-0026 v4, the registrar owner remains the sole signer for ProRegTx
and ProUpRegTx, and may freely rewrite the payout list at any time.

This DIP does not modify DIP-0026 v4 semantics. Shared masternodes use a new
provider transaction payload version, `5` (`SharedCollateral`), with the

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Blocking: Provider payload versioning conflicts with the referenced DIP-0026

This draft normatively claims DIP-0026 introduces provider transaction payload version 4 (MultiPayout) and then allocates SharedCollateral = 5 for v5. The DIP-0026 document checked into this repository (dip-0026.md line 46) says it introduces version 2 of ProRegTx/ProUpRegTx with payoutShares/payoutSharesSize, and makes no claim about versions 3 or 4. As a result, implementers reading only the merged DIPs cannot determine whether DIP-0026 is v2 or v4, what occupies version 3, or whether SharedCollateral should actually be version 3 (next free after DIP-0026) rather than version 5.

The provider payload version is a consensus selector, so this is the kind of registry mismatch the review guidance flags as blocking. Either amend DIP-0026 in this repo to match (renumber its payload to v4 and reserve v3 / specify what it is), or change this DIP's version allocation to be consistent with DIP-0026 as merged. The 'Open Issues' section does not currently cover this dependency, so add it there or in 'Relationship to DIP-0026' with the resolution path.

source: ['codex']

Comment thread dip-decentralized-masternode-shares.md Outdated
Comment on lines +566 to +568
4. `newRewardScript` MUST satisfy the same key-reuse restrictions as
registration: it MUST NOT be a P2PKH paying to any
`shares[i].ownerKey` and MUST NOT pay to `keyIDVoting`.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💬 Nitpick: ProUpShareTx key-reuse rule wording is inconsistent with registration rule 8

Registration rule 8 (lines 373–377) restricts refundScript/rewardScript from being a P2PKH script paying to the listed key IDs. ProUpShareTx rule 4 reads it MUST NOT be a P2PKH paying to any shares[i].ownerKey and MUST NOT pay to keyIDVoting. The second clause drops the P2PKH qualifier, so it can be read either as more restrictive (any script form paying to keyIDVoting, which is not well-defined for non-P2PKH templates) or as a wording slip. Aligning the wording with registration rule 8 removes the ambiguity for the consensus check.

Suggested change
4. `newRewardScript` MUST satisfy the same key-reuse restrictions as
registration: it MUST NOT be a P2PKH paying to any
`shares[i].ownerKey` and MUST NOT pay to `keyIDVoting`.
4. `newRewardScript` MUST satisfy the same key-reuse restrictions as
registration: it MUST NOT be a P2PKH paying to any
`shares[i].ownerKey` and MUST NOT be a P2PKH paying to `keyIDVoting`.

source: ['claude']

Comment thread dip-decentralized-masternode-shares.md Outdated
Comment on lines +1443 to +1444
4. A reorg that replays a `ProUpSharedRegTx` followed by a `ProDisTx`
in a different order produces the same final state.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💬 Nitpick: Reorg test case 4 description is logically inconsistent

Test case 4 says A reorg that replays a ProUpSharedRegTx followed by a ProDisTx in a different order produces the same final state. Read literally, the two orders are [ProUpSharedRegTx, ProDisTx] and [ProDisTx, ProUpSharedRegTx]. Under this spec a valid ProDisTx removes the masternode entry, so the subsequent ProUpSharedRegTx against the same proTxHash would be invalid (the masternode no longer exists). The two orders cannot both produce the same successful final state. Reword so the test target is unambiguous (e.g., independent updates on disjoint masternodes interleaved across a reorg, or that disconnect-then-reconnect reproduces the original deterministic state regardless of intra-block ordering).

source: ['claude']

@thepastaclaw

Copy link
Copy Markdown
Author

@coderabbitai review

Comment thread dip-decentralized-masternode-shares.md Outdated
independent `nVersion` field that starts at `1` (see each payload
definition).

Provider payload version `5` is reserved for shared-collateral semantics.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this is a poor decision. It seems to be that we should be able to have a specific individual version to serve both shared and non-shared masternodes. Either we have a flag which indicates shared, or we detect it based on length of the owners or something where only 1 shows it's not shared.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acknowledged. I removed the paragraph that tried to reserve v5/future versioning as a generic-mode decision. I left the current draft using separate SharedCollateral = 5, but called out in the PR description that this is open design feedback to resolve before ready-for-review: separate provider payload version vs one version with an explicit shared flag or share-count discriminator. I did not silently redesign that part without a concrete direction.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am saying this IS the design decision. I do not wish 1 version to mean semantically shared and another to mean not. Instead a flag or something else such as length of scripts should say that

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 1b778bc by removing the new ProRegTx provider payload version and making shared collateral a v4 mode selected by an explicit isSharedCollateral discriminator. The draft now states that non-shared v4 remains governed by DIP-0026, shared behavior gates on state.isSharedCollateral, and the registration consent digest commits to the discriminator.

Comment thread dip-decentralized-masternode-shares.md Outdated
| `TRANSACTION_PROVIDER_UPDATE_SHARE` | 11 | `ProUpShareTx` payload (this DIP). |
| `TRANSACTION_PROVIDER_UPDATE_SHARED_REGISTRAR` | 12 | `ProUpSharedRegTx` payload (this DIP). |

The numeric assignments `10`, `11`, and `12` are tentative. Before this DIP

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't have hedging paragraphs like this. For stuff similar to this keep it in the PR description not in the dip.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done — moved the dependency/merge-blocking prose out of the DIP and into the PR description. The DIP now states the DIP-0026 v4 relationship directly without the PR-status hedging.

thepastaclaw and others added 11 commits June 20, 2026 07:40
Introduces a draft DIP describing shared-collateral masternodes with
2-8 participants, ProTxVersion 5 (SharedCollateral), per-participant
consent for registration and shared-registrar updates, consensus
enforcement of a dissolution covenant on the shared collateral output,
and a new CProDisTx (TRANSACTION_PROVIDER_DISSOLVE) with strict
output/penalty rules. Filed under the alias name per DIP editor policy.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* Replace 1051200 (~2 yr at 60 s) early-period ceiling with 420480
  (~2 yr at nominal 2.5 min/block) and document wall-clock conversion.
* Narrow the shared collateral covenant: consensus protects only
  active or same-block-pending v5 collateral outpoints, not every
  output whose scriptPubKey matches the template, so unrelated UTXOs
  with the same script are not retroactively locked.
* Fix DistributeByWeight remainder distribution to skip zero-weight
  indices and require sum_w > 0, and add helper tests.
* Allow the actor's dissolution output to be omitted when its value
  would be zero (e.g. when the fee consumes the full pre-fee
  remainder); reject stray zero-value actor outputs; specify output
  matching by walking the share table.
* Special-case unanimous dissolution so DistributeByWeight is not
  invoked with an all-zero weight vector; bonus is set to zero
  directly.
* Update test cases and penalty-bounds rationale.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Address compatibility/versioning review on the draft Shared Masternode
DIP:

* Bind SharedRegConsentHash and SharedDisHash to chainGenesisHash so
  consensus digests are network-scoped; signers MUST verify chain
  context before signing. Rewrite the cross-chain replay security
  consideration accordingly: proTxHash alone is not sufficient.
* Clarify that ProTxVersion::SharedCollateral = 5 is the v5 ProRegTx
  payload version and the shared-MN state tag; the new ProDisTx,
  ProUpShareTx, and ProUpSharedRegTx payloads carry their own
  independent nVersion starting at 1. Reserve future provider payload
  versions (6+) for single-owner ProRegTx changes; v5 is not a
  generic mode flag.
* Clean up ProUpRegTx/ProUpServTx wording: ProUpRegTx is invalid
  against a v5 masternode; ProUpServTx remains operator-authorized for
  service/operator-payout fields only and MUST NOT touch owner or
  shared-collateral fields.
* Spell out the SML/filter trust boundary: SML hashes MUST NOT include
  v5-only fields, full nodes validate v5 data from reconstructed state,
  filter matches exist for client discovery only and are not consensus
  commitments. A future DIP-0004 extension would be required for SPV
  commitments. Mirrors the non-commitment direction taken for DIP-0026
  payout metadata.
* Mark the DIP explicitly non-final for activation until concrete
  SHARED_COLLATERAL_SCRIPT bytes are assigned and inserted, and
  strengthen the open issue requiring re-check of the tentative
  10/11/12 special transaction type numbers against the authoritative
  DIP-0002 / Dash Core allocations before merge or activation, with
  mandatory renumbering on conflict.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…re-vector diffs

Address consensus review blockers ahead of activation-final:

* Deterministic MN state diffs: remove the MAY-compact alternative and
  require a single canonical encoding - full replacement of the share
  vector on any share-field change.
* Same-block ProRegTx + ProDisTx: forbid dissolution in the same block
  as the v5 registration. The protected set is split into an active
  set (allows ProDisTx) and a same-block pending set (rejects every
  spend, including ProDisTx). Update covenant rules, rationale,
  implementation notes, and test cases accordingly.
* Activation language: tighten draft-status wording so the DIP is
  reviewable but explicitly not activation-final until script bytes
  and DIP-0023 deployment parameters are assigned. No specific Dash
  Core release version is claimed.
@thepastaclaw thepastaclaw force-pushed the docs/dip-decentralized-mn-shares branch from 1550555 to c14a4cd Compare June 20, 2026 12:41
@coderabbitai

coderabbitai Bot commented Jun 20, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@thepastaclaw thepastaclaw force-pushed the docs/dip-decentralized-mn-shares branch from c14a4cd to b64a580 Compare June 20, 2026 12:43

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Draft DIP for decentralized masternode shares (ProTx v5 + ProDisTx/ProUpShareTx/ProUpSharedRegTx) is tight on the covenant model, digests, penalty bounds, same-block dissolution, and reorg replay. One blocking spec ambiguity remains: the covenant uses the undefined term "active v5 masternode" which could let PoSe-banned masternode collateral escape mempool/block-level rejection if implementations interpret "active" as the valid (non-banned) subset rather than the registered set.

🔴 1 blocking

🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `dip-decentralized-masternode-shares.md`:
- [BLOCKING] dip-decentralized-masternode-shares.md:957-961: Define "active v5 masternode" to include PoSe-banned registrations
  The active-set definition keys the mempool and block-level covenant rejection on "every collateral outpoint recorded against an active v5 masternode in the deterministic masternode list", but "active" is not defined. DIP-0003 distinguishes the registered set from the valid (non-PoSe-banned) subset, and an implementer reasonably reading "active" as "valid" would exclude PoSe-banned v5 masternodes from the active set. Mempool rule 1 (line 985) and block-connection rule 2 (line 994-1000) then would not reject an ordinary spend of a PoSe-banned v5 masternode's shared collateral, even though the later list-removal rule 4 (line 1009-1016) says any v5 masternode must only be removed by ProDisTx. That mismatch means a PoSe-banned masternode could be removed via collateral spend on one implementation and rejected on another — a consensus split. Same ambiguity affects the parallel summary at lines 264-269 and references to "active v5 masternode" at lines 1455, 1458, 1476. Specify that the protected active set is every registered v5 masternode in the deterministic list, including PoSe-banned ones, then propagate the wording consistently.

GitHub does not allow PastaClaw to request changes on their own PR, so this is posted as a COMMENT review while preserving the blocking finding.

Inline posting also hit GitHub HTTP 422, so I posted the same verified finding as a top-level review body.

@thepastaclaw

Copy link
Copy Markdown
Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 21, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Cumulative incremental review of PR #185 at f6bd78a. Prior b64a580 finding (active-vs-registered v5 masternode set, PoSe-banned membership) is FIXED at f6bd78a — the protected-set sections, collateral-spend enforcement, and Test 17 all now use 'registered' and explicitly include PoSe-banned entries until removal by a valid ProDisTx. One new latest-delta finding: the early-period prose at line 150 and the consensus formula at line 817 disagree on how many blocks the early penalty actually covers once same-block dissolution is forbidden, which leaks into consensus-binding ProDisTx output amounts. CodeRabbit posted only a processing summary with zero inline findings, so coderabbit_reactions is empty.

🔴 1 blocking

🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `dip-decentralized-masternode-shares.md`:
- [BLOCKING] dip-decentralized-masternode-shares.md:817-818: Early-period formula and prose disagree on the size of the early window
  The terminology entry at line 150 defines the early period as 'The window of `earlyPeriodBlocks` blocks after registration during which `earlyPenalty` applies.' The consensus formula at line 817 is `early = (H - state.nRegisteredHeight) < state.earlyPeriodBlocks`. However, same-block dissolution is explicitly forbidden (lines 925-936, 1302-1310, 1465-1471), so the first connectable `ProDisTx` is at `H = state.nRegisteredHeight + 1`. With the strict `<` test, the early-penalty regime then covers `earlyPeriodBlocks - 1` eligible blocks rather than `earlyPeriodBlocks`, and the edge case `earlyPeriodBlocks == 1` covers zero eligible blocks despite the prose promising a one-block window. Because `penalty` directly sets the consensus-required non-actor refund amounts and the actor's maximum output (lines 1421-1432), an implementer who follows the prose (`H - nRegisteredHeight <= earlyPeriodBlocks` or counts the natural N blocks after registration) will accept different `ProDisTx` outputs than one who follows the formula. Pick one semantics and align both the prose and the formula (and document the `earlyPeriodBlocks == 0` and `== 1` degenerate cases) before activation.

GitHub does not allow PastaClaw to request changes on their own PR, so this is posted as a COMMENT review while preserving the blocking finding.

Inline/request-changes posting hit GitHub HTTP 422, so I posted the same verified finding as a top-level review body.

@thepastaclaw

Copy link
Copy Markdown
Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 21, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

The prior f6bd78a early-period prose/formula mismatch is FIXED at dcd170b: the terminology entry now defines the early period in terms of eligible dissolution blocks (excluding same-block dissolution and explicitly disabling the window when earlyPeriodBlocks == 0), and the selector at lines 817-819 is gated on earlyPeriodBlocks > 0 with <=, matching the prose. One carried-forward latest-delta-adjacent docs nit remains in the Reorg test list where 'in a different order' is ambiguous against the existence precondition for ProUpSharedRegTx. No new blocking issues.

🟡 1 suggestion(s)

🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `dip-decentralized-masternode-shares.md`:
- [SUGGESTION] dip-decentralized-masternode-shares.md:1498-1499: Reorg test #4 wording conflicts with the update-target-must-exist rule
  Test 4 states that replaying a `ProUpSharedRegTx` followed by a `ProDisTx` 'in a different order' produces the same final state. Read literally, swapping the order places the `ProDisTx` first, which under the normative rules at lines 615-616 (the masternode identified by `proTxHash` MUST exist and MUST be a v5 masternode) and the dissolution-removes-the-entry semantics makes the later `ProUpSharedRegTx` invalid because the masternode no longer exists. The intent appears to be disconnect/reconnect determinism along the original chain order, not a branch with reversed transaction ordering. Tighten the wording so implementers do not infer that order-swapped reorg branches must converge.

Comment thread dip-decentralized-masternode-shares.md Outdated
@thepastaclaw

Copy link
Copy Markdown
Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 21, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Latest delta (dcd170b..7c91aa5) is a single targeted wording fix that resolves the prior reorg-test #4 ordering contradiction by preserving original chain order. Cumulative draft remains under-specified in two places: the on-wire framing of the top-level share vector in the v5 ProRegTx payload, and the state-diff bit value for the share-vector field (neither assigned nor listed in Open Issues). Both are suggestion-grade improvements for a draft; no blocking defects.

🟡 2 suggestion(s)

🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `dip-decentralized-masternode-shares.md`:
- [SUGGESTION] dip-decentralized-masternode-shares.md:386: Specify on-wire framing for the share vector
  The v5 ProRegTx layout claims to be exhaustive and normative for `nVersion == 5`, but the `shares` row only says `CollateralShare[]` without naming the vector's length prefix. Every other vector in this DIP — `refundScript`, `rewardScript`, `joinSig`, and `vchSigs` — explicitly calls out `compact-size length-prefixed`, and the registration consent digest separately uses `LE8(shares.size())`. A reader synthesising the payload framing from those two sources alone could plausibly serialize the outer count as either CompactSize or LE8. CompactSize is the Dash/Bitcoin convention for top-level vectors, but the asymmetry with neighbouring vector fields is the kind of ambiguity the spec elsewhere takes care to remove. Add an explicit `sharesSize` row (or an inline note that `CollateralShare[]` is a CompactSize-prefixed vector) so the byte boundary preceding `earlyPeriodBlocks` is unambiguous.
- [SUGGESTION] dip-decentralized-masternode-shares.md:1070-1079: Assign the share-vector state-diff bit or mark it as a pre-activation open item
  This clause makes the share-vector diff encoding normative (full replacement is the single canonical representation), but it only says the state-diff bitfield 'gains a new field bit' without assigning the bit value. The state-diff bitfield is a consensus selector for how serialized deterministic-MN state diffs are decoded, so leaving the value unspecified prevents implementers from independently producing interoperable snapshots and reorg-replay payloads. The Open Issues section (line 1634) already enumerates other pre-activation deferred items (SHARED_MIN_SHARE_DUFFS, activation deployment name) — either assign the exact bit value here or add an explicit entry to Open Issues so the gap is tracked.

Comment thread dip-decentralized-masternode-shares.md Outdated
Comment thread dip-decentralized-masternode-shares.md Outdated
@thepastaclaw

Copy link
Copy Markdown
Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 21, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

The 113b147 delta correctly fixes both prior 7c91aa5 findings: the v5 ProRegTx shares row at line 386 now specifies it as a CompactSize-prefixed vector (resolving the byte-framing ambiguity before earlyPeriodBlocks), and the new state-diff bit for the share vector is explicitly deferred at lines 1070-1071 with a matching pre-activation Open Issues entry at lines 1645-1646. No new in-scope spec defects remain; the draft is internally consistent with its DIP: TBD posture and the proposed DIP-0002 registry rows for types 10/11/12.

GitHub does not allow PastaClaw to approve or request changes on their own PR, so this is posted as a COMMENT review rather than APPROVE.

@thepastaclaw thepastaclaw marked this pull request as ready for review June 21, 2026 22:35
@thepastaclaw

Copy link
Copy Markdown
Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 21, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (1)
dip-decentralized-masternode-shares.md (1)

45-67: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Align the DIP-0026 version reference before this draft lands.

This section still hardcodes DIP-0026 v4, but the checked-in dip-0026.md in this branch describes the multi-payout payload as version 2. Until the companion update is part of the same stack, readers have two conflicting version contracts and can’t tell which payload layout to implement.

Also applies to: 115-123

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@dip-decentralized-masternode-shares.md` around lines 45 - 67, The document
contains hardcoded references to DIP-0026 v4, but the actual dip-0026.md file
describes the multi-payout payload as version 2, creating conflicting version
contracts for readers. Locate all instances where DIP-0026 is referenced with a
version number in dip-decentralized-masternode-shares.md (including the sections
around lines 45-67 and 115-123 as indicated in the comment), verify the correct
version from the dip-0026.md file in the branch, and update all version
references to match consistently so readers have a single authoritative version
contract to implement against.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Duplicate comments:
In `@dip-decentralized-masternode-shares.md`:
- Around line 45-67: The document contains hardcoded references to DIP-0026 v4,
but the actual dip-0026.md file describes the multi-payout payload as version 2,
creating conflicting version contracts for readers. Locate all instances where
DIP-0026 is referenced with a version number in
dip-decentralized-masternode-shares.md (including the sections around lines
45-67 and 115-123 as indicated in the comment), verify the correct version from
the dip-0026.md file in the branch, and update all version references to match
consistently so readers have a single authoritative version contract to
implement against.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: cfe3be39-7f94-4c84-973d-02876210436c

📥 Commits

Reviewing files that changed from the base of the PR and between 113b147 and 1b778bc.

📒 Files selected for processing (1)
  • dip-decentralized-masternode-shares.md

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

The latest delta replaces a separate shared provider payload version with an isSharedCollateral discriminator embedded inside the existing DIP-0026 v4 ProRegTx payload. The prior CompactSize share-vector framing and deferred state-diff-bit concerns from 7c91aa5 remain resolved at 1b778bc. One blocking spec-correctness issue remains: the discriminator byte's position is normatively defined here but not reconciled with the pending DIP-0026 v4 wire layout, leaving the v4 framing ambiguous across shared vs non-shared parsers.

GitHub does not allow PastaClaw to approve or request changes on their own PR, so this is posted as a COMMENT review rather than REQUEST_CHANGES.

🔴 1 blocking

🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `dip-decentralized-masternode-shares.md`:
- [BLOCKING] dip-decentralized-masternode-shares.md:223-244: Discriminator byte position must be reserved by DIP-0026 v4 or moved to its own payload version
  This section makes `isSharedCollateral` a `uint8_t` placed in the v4 ProRegTx payload after `nMode` (Section 5.6 confirms the position at line 428-429), and then states that `isSharedCollateral == 0x00` payloads `follow the unchanged DIP-0026 multi-payout layout` (line 232-236) and `deserialize and validate exactly as DIP-0026 specifies` (line 1330-1333). DIP-0026 v4 is itself a pending/unmerged dependency (acknowledged at line 168 as the `pending DIP-0026 v4 design`), and nothing in this DIP or in the current repository copy of DIP-0026 records that DIP-0026 v4 reserves a discriminator byte at that position. As written, a shared-mode parser consumes one byte after `nMode` to select the variant, while a DIP-0026 v4 non-shared parser would treat that same byte as the first byte of `collateralOutpoint.hash`. The two parsers therefore frame the same v4 bytes differently, which shifts the rest of the payload for non-shared v4 registrations, invalidates payload hashes/signatures across implementations, and turns valid external-collateral hashes that happen to begin with a byte other than `0x00` into illegal discriminator values. Before activation, either DIP-0026 v4 must explicitly reserve this discriminator byte at the same wire offset for all v4 ProRegTx payloads (and that coordination must be called out here as an unresolved cross-DIP dependency in `Open Issues`), or shared collateral must be carried by a distinct provider payload version so the wire-format selector is unambiguous. The current `Open Issues` section does not list this coordination, and the rationale at line 1317-1333 argues against a separate version without addressing the framing prerequisite.

Comment thread dip-decentralized-masternode-shares.md Outdated
Comment on lines +223 to +244
The discriminator is a single `uint8_t` field in the v4 ProRegTx payload:

| Name | Type | Encoding |
| --- | --- | --- |
| `isSharedCollateral` | `uint8_t` | `0x00` for non-shared (DIP-0026 multi-payout) mode; `0x01` for shared-collateral mode. Any other value is invalid. |

The discriminator selects mutually exclusive variant fields in the same v4
payload:

* `isSharedCollateral == false`: the v4 payload follows the unchanged
DIP-0026 multi-payout layout (single `keyIDOwner`, `scriptPayout` /
`payouts`, no share table, no shared collateral output, no penalty
parameters, no `earlyPeriodBlocks`). All DIP-0026 v4 semantics, validation,
state, and authorization apply verbatim and are unaffected by this DIP.
* `isSharedCollateral == true`: the v4 payload follows the shared-collateral
layout defined in [Shared Registration (ProRegTx v4, shared
mode)](#shared-registration-proregtx-v4-shared-mode) (no `keyIDOwner`, no
`scriptPayout`, no `payouts`, a `CollateralShare[]` table, an internal
collateral output that pays `SHARED_COLLATERAL_SCRIPT`, and per-masternode
penalty parameters). The shared-collateral variant is selected solely by
the discriminator; consensus MUST NOT infer the variant from any other
field length, script shape, or output count.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Blocking: Discriminator byte position must be reserved by DIP-0026 v4 or moved to its own payload version

This section makes isSharedCollateral a uint8_t placed in the v4 ProRegTx payload after nMode (Section 5.6 confirms the position at line 428-429), and then states that isSharedCollateral == 0x00 payloads follow the unchanged DIP-0026 multi-payout layout (line 232-236) and deserialize and validate exactly as DIP-0026 specifies (line 1330-1333). DIP-0026 v4 is itself a pending/unmerged dependency (acknowledged at line 168 as the pending DIP-0026 v4 design), and nothing in this DIP or in the current repository copy of DIP-0026 records that DIP-0026 v4 reserves a discriminator byte at that position. As written, a shared-mode parser consumes one byte after nMode to select the variant, while a DIP-0026 v4 non-shared parser would treat that same byte as the first byte of collateralOutpoint.hash. The two parsers therefore frame the same v4 bytes differently, which shifts the rest of the payload for non-shared v4 registrations, invalidates payload hashes/signatures across implementations, and turns valid external-collateral hashes that happen to begin with a byte other than 0x00 into illegal discriminator values. Before activation, either DIP-0026 v4 must explicitly reserve this discriminator byte at the same wire offset for all v4 ProRegTx payloads (and that coordination must be called out here as an unresolved cross-DIP dependency in Open Issues), or shared collateral must be carried by a distinct provider payload version so the wire-format selector is unambiguous. The current Open Issues section does not list this coordination, and the rationale at line 1317-1333 argues against a separate version without addressing the framing prerequisite.

source: ['codex']

@thepastaclaw

Copy link
Copy Markdown
Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 21, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Both agent reviews and the prior discriminator/framing concern were reconciled against the current head 51ec7ac. The discriminator byte position is explicitly stated (line 224, immediately after nMode) and the cross-DIP dependency is now captured as Open Issue #3 (lines 1805-1809), which is acceptable for a draft per the repo review skill. No in-scope blocking, suggestion, or nitpick defects remain; previously deferred items (SHARED_MIN_SHARE_DUFFS, deployment name, share-vector state-diff bit) are still appropriately listed under Open Issues.

GitHub does not allow PastaClaw to approve or request changes on their own PR, so this is posted as a COMMENT review rather than APPROVE.

@thepastaclaw

Copy link
Copy Markdown
Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 22, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants